perf: use vector instead CDeterministicMNList for computing rotating quorums#6678
perf: use vector instead CDeterministicMNList for computing rotating quorums#6678PastaPastaPasta merged 7 commits intodashpay:developfrom
Conversation
WalkthroughThe changes remove the quorum scoring and selection methods ( 📜 Recent review detailsConfiguration used: CodeRabbit UI 📒 Files selected for processing (1)
🚧 Files skipped from review as they are similar to previous changes (1)
⏰ Context from checks skipped due to timeout of 90000ms (9)
✨ Finishing Touches
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. 🪧 TipsChatThere are 3 ways to chat with CodeRabbit:
SupportNeed help? Create a ticket on our support page for assistance with any issues or questions. Note: Be mindful of the bot's finite context window. It's strongly recommended to break down tasks such as reading entire modules into smaller chunks. For a focused discussion, use review comments to chat about specific files and their changes, instead of using the PR comments. CodeRabbit Commands (Invoked using PR comments)
Other keywords and placeholders
CodeRabbit Configuration File (
|
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (2)
src/llmq/utils.cpp (2)
168-181: Sorting trick works but is brittle – consider a clearer approachUsing
std::sort(scores.rbegin(), scores.rend(), comp)with a comparator that
orders bya.first < b.firstgives the desired descending order in the
original container, but the double indirection (reverse-iterators and an
ascending comparator) is non-obvious and easy to break during refactors.A more idiomatic and self-documenting alternative:
- std::sort(scores.rbegin(), scores.rend(), + std::sort(scores.begin(), scores.end(), [](const auto& a, const auto& b) { - ... - return a.first < b.first; + ... + return a.first > b.first; // explicit descending });Same complexity, easier for future maintainers to reason about.
108-730: Code style fails clang-format – please run the formatterThe CI pipeline reports
Clang format differences found.
Runclang-format -i src/llmq/utils.cpp(or the repository wrapper script)
and commit the resulting changes to keep the file consistent with the project
style guide.🧰 Tools
🪛 GitHub Actions: Clang Diff Format Check
[error] 108-730: Clang format differences found. The file does not adhere to the required code style. Please run clang-format to fix formatting issues.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
Cache: Disabled due to data retention organization setting
Knowledge Base: Disabled due to data retention organization setting
📥 Commits
Reviewing files that changed from the base of the PR and between dc6c35b and 622d488ac669f935e8e1dff07f30ca5574974144.
📒 Files selected for processing (3)
src/evo/deterministicmns.cpp(0 hunks)src/evo/deterministicmns.h(0 hunks)src/llmq/utils.cpp(10 hunks)
💤 Files with no reviewable changes (2)
- src/evo/deterministicmns.cpp
- src/evo/deterministicmns.h
🧰 Additional context used
🪛 GitHub Actions: Clang Diff Format Check
src/llmq/utils.cpp
[error] 108-730: Clang format differences found. The file does not adhere to the required code style. Please run clang-format to fix formatting issues.
⏰ Context from checks skipped due to timeout of 90000ms (12)
- GitHub Check: linux64_ubsan-build / Build source
- GitHub Check: build (x86_64-apple-darwin)
- GitHub Check: build (powerpc64-linux-gnu)
- GitHub Check: build (arm64-apple-darwin)
- GitHub Check: build (riscv64-linux-gnu)
- GitHub Check: build (x86_64-w64-mingw32)
- GitHub Check: build (arm-linux-gnueabihf)
- GitHub Check: build (x86_64-linux-gnu)
- GitHub Check: build (aarch64-linux-gnu)
- GitHub Check: x86_64-apple-darwin / Build depends
- GitHub Check: x86_64-pc-linux-gnu_multiprocess / Build depends
- GitHub Check: arm-linux-gnueabihf / Build depends
🔇 Additional comments (1)
src/llmq/utils.cpp (1)
694-727: Snapshot size is assumed, not validated
snapshot.activeQuorumMembers[i]is accessed for every element in
sortedAllMns, implicitly assuming that
snapshot.activeQuorumMembers.size() == sortedAllMns.size().
If the snapshot is ever malformed (e.g. from disk corruption or a logic error)
this will trigger OOB reads and UB.Consider an explicit bounds check or an
assert:assert(snapshot.activeQuorumMembers.size() == sortedAllMns.size());and gracefully abort if the sizes diverge at runtime on release builds.
Co-authored-by: UdjinM6 <UdjinM6@users.noreply.github.com>
… quorum calculation
| template <typename List> | ||
| static std::vector<CDeterministicMNCPtr> CalculateQuorum(List&& mn_list, const uint256& modifier, size_t maxSize = 0, |
There was a problem hiding this comment.
It seems to me better to make this accept a Span so that we have non-templated types
There was a problem hiding this comment.
auto allMns = dmnman.GetListForBlock(pWorkBlockIndex);
return CalculateQuorum(allMns, modifier, llmq_params_opt->size, EvoOnly);
allMns has type CDeterministicMNList and there's template to be sure that it works with both types: std::vector<CDeterministicMNCPtr and CDeterministicMNList.
To avoid conversion from CDeterministicMNList to std::vector (which is prety expensive operation) there's used a template param.
I don't see an easy [cheap] way to convert CDeterministicMNList to vector.
There was a problem hiding this comment.
well, why can CDeterministicMNList be span compatible?
…apshot Co-authored-by: PastaPastaPasta <6443210+PastaPastaPasta@users.noreply.github.com>
… computing rotating quorums 2389d97 refactor: remove un-needed std::move from GetQuorumQuarterMembersBySnapshot (Konstantin Akimov) 7ab8647 perf: move std::vector of masternodes instead copying during rotation quorum calculation (Konstantin Akimov) 026847a refactor: add default argument maxSize for CalcualeQuorum (Konstantin Akimov) 291ea2e refactor: remove useless variable copy (even with move) (Konstantin Akimov) fd901e0 perf: futher using std::vector instead CDeterministicMNList (Konstantin Akimov) 80faf1b perf: use vector instead DeterministicMNList for calculation of rotating quorums (Konstantin Akimov) 77273d5 perf: use std::move during quorum calculation (Konstantin Akimov) Pull request description: ## Issue being fixed or feature implemented Helper to get quorum members uses CDeterministicMNList as a source of data. It makes temporary calculation slower, because temporary lists of masternodes should not construct heavy objects CDeterministicMNList just to keep a list of them. ## What was done? - helpers `CalculateQuorum`, `CalculateScores` are moved from class `CDeterministicMNList` to `llmq::utils` so far as they don't actually use any private data of `CDeterministicMNList` - helper `CalculateQuorum` can accept as source of data not only CDeterministicMNList but std::vector with shared ptrs. - helper `BuildNewQuorumQuarterMembers` does not use CDeterministicMNList for `MnsNotUsedAtH` - `GetMNUsageBySnapshot` does not use CDeterministicMNList anymore Also, `BuildNewQuorumQuarterMembers` uses `std::move` when possible. ## How Has This Been Tested? invalidate + reconsider 15000 blocks; check logs and `perf`. By perf `PreComputeQuorumMembers` got almost double faster; most improvements came from `GetQuorumQuarterMembersBySnapshot` as expected. It should give roughly 2% overall improvement for block validation speed and reindex. PR: <img width="747" alt="image" src="https://github.com/user-attachments/assets/d27778fa-cc09-4bd7-bccd-601fa6df75f3" /> ``` 2025-05-20T15:46:53Z [bench] - m_qblockman: 0.05ms [87.08s] 2025-05-20T15:46:53Z [bench] - Connect block: 21.17ms [249.92s (17.38ms/blk)] ``` Develop: <img width="747" alt="image" src="https://github.com/user-attachments/assets/aff54059-703d-4c76-967a-15279b1b420c" /> ``` 2025-05-20T16:11:33Z [bench] - m_qblockman: 0.05ms [93.42s] 2025-05-20T16:11:33Z [bench] - Connect block: 19.06ms [256.95s (17.13ms/blk)] ``` ## Breaking Changes N/A ## Checklist: - [x] I have performed a self-review of my own code - [ ] I have commented my code, particularly in hard-to-understand areas - [ ] I have added or updated relevant unit/integration/functional/e2e tests - [ ] I have made corresponding changes to the documentation - [x] I have assigned this pull request to a milestone ACKs for top commit: UdjinM6: utACK 2389d97 PastaPastaPasta: utACK 2389d97 Tree-SHA512: 86db57e74eb815cc030421610d20e2dd90dfd18fcf429f97ba96681b69539cce54b0f4c9146e29beb541d1e0193919a1242bd08cba8df7017cfe4b42b8be7a63
3 participants
Issue being fixed or feature implemented
Helper to get quorum members uses CDeterministicMNList as a source of data.
It makes temporary calculation slower, because temporary lists of masternodes should not construct heavy objects CDeterministicMNList just to keep a list of them.
What was done?
CalculateQuorum,CalculateScoresare moved from classCDeterministicMNListtollmq::utilsso far as they don't actually use any private data ofCDeterministicMNListCalculateQuorumcan accept as source of data not only CDeterministicMNList but std::vector with shared ptrs.BuildNewQuorumQuarterMembersdoes not use CDeterministicMNList forMnsNotUsedAtHGetMNUsageBySnapshotdoes not use CDeterministicMNList anymoreAlso,
BuildNewQuorumQuarterMembersusesstd::movewhen possible.How Has This Been Tested?
invalidate + reconsider 15000 blocks; check logs and
perf.By perf
PreComputeQuorumMembersgot almost double faster; most improvements came fromGetQuorumQuarterMembersBySnapshotas expected.It should give roughly 2% overall improvement for block validation speed and reindex.
PR:
Develop:
Breaking Changes
N/A
Checklist: